[naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped #5227

Imberflur · 2024-02-09T02:55:49Z

Connections
Fixes #4915

Description
Unused gl_PerVertex builtins need to be skipped since they my otherwise require capabilities that are not enabled. However, this was not being done when the input spirv didn't explicitly name this struct.

This just removes the check against the "gl_PerVertex" name as suggested in #4915 (comment). Assuming valid SPIRV, this is technically equivalent to #4915 (comment) because a struct with builtin members must be decorated as a Block

Would it be worth adding a test for this? If so, would that go under naga/tests/in/spv?

Also, I'm not very familiar with the overall structure of SPIRV nor with the spv frontend for naga. Can parse_function run more than once for a particular input? If so, is it possible that later functions will be using the builtin but we remove the builtin label after not seeing any uses when processing the first function?

Testing
I produced a .spv file using the method described in #4915 and confirmed that this change removes the error there. I also tested an application using shaderc to generate SPIRV for wgpu that was seeing this error with certain configurations of shaderc and confirmed that the issue was resolved (and that everything seemed to work).

Checklist

Run cargo fmt.
Run cargo clippy. If applicable, add:
- ~~--target wasm32-unknown-unknown~~
- ~~--target wasm32-unknown-emscripten~~

Run cargo xtask test to run tests.

Some failures but they also fail on `trunk`

Summary [   6.158s] 411 tests run: 406 passed (5 flaky), 5 failed, 0 skipped
     FLAKY 2/3 [   0.251s] wgpu-examples [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_examples::hello_compute::tests::compute_1
     FLAKY 2/3 [   0.374s] wgpu-examples [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_examples::hello_compute::tests::compute_overflow
     FLAKY 2/3 [   0.271s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::occlusion_query::occlusion_query
     FLAKY 2/3 [   0.236s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::regression::issue_3457::pass_reset_vertex_buffer
     FLAKY 2/3 [   0.278s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::shader_primitive_index::draw_indexed
    TRY 3 SEGV [   0.727s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::mem_leaks::simple_draw_check_mem_leaks
    TRY 3 SEGV [   0.579s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::regression::issue_3349::multi_stage_data_binding
    TRY 3 SEGV [   0.552s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::scissor_tests::scissor_test_empty_rect
    TRY 3 SEGV [   0.504s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::scissor_tests::scissor_test_empty_rect_with_offset
    TRY 3 SEGV [   0.569s] wgpu-test::wgpu-test [Executed] [Gl/AMD Radeon RX 6500 XT (radeonsi, navi24, LLVM 16.0.6, DRM 3.57, 6.7.4-arch1-1)/1] wgpu_test::scissor_tests::scissor_test_full_rect

Add change to CHANGELOG.md. See simple instructions inside file.

Edit:

Additional changes

Removes perVertexStruct naming as requested below [naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped #5227 (comment). For reference, this was originally added in [spv-in] Properly parse empty names and set entry point function name naga#789.
Entry point specific code from parse_function was extracted and now runs after all functions are parsed. Since this includes the code that does gl_PerVertex builtin culling, we can be sure that any potential access has been registered.

naga/src/front/spv/function.rs

teoxoy · 2024-02-09T14:24:29Z

This looks good, could we also take out the block below? I don't think we should be naming things if they don't have a name in the input.

wgpu/naga/src/front/spv/mod.rs

Lines 5130 to 5139 in 665c075

    
           // Fix empty name for gl_PerVertex struct generated by glslang 
        
           if let crate::TypeInner::Pointer { .. } = module.types[original_ty].inner { 
        
               if ext_class == ExtendedClass::Input || ext_class == ExtendedClass::Output { 
        
                   if let Some(ref dec_name) = dec.name { 
        
                       if dec_name.is_empty() { 
        
                           dec.name = Some("perVertexStruct".to_string()) 
        
                       } 
        
                   } 
        
               } 
        
           }

teoxoy · 2024-02-09T14:32:58Z

Would it be worth adding a test for this?

That would be great, thanks!

If so, would that go under naga/tests/in/spv?

Yes.

Also, I'm not very familiar with the overall structure of SPIRV nor with the spv frontend for naga. Can parse_function run more than once for a particular input?

SPIR-V does support multiple entry points per file but they are not common.

If so, is it possible that later functions will be using the builtin but we remove the builtin label after not seeing any uses when processing the first function?

This is a good point, we might need to defer the removal.

Imberflur · 2024-02-11T19:51:40Z

This is a good point, we might need to defer the removal.

This seems to be the case, I was able to get spv-in to remove the builtin from position by moving positing setting out of the main function:

#version 450

out gl_PerVertex {
    vec4 gl_Position;
    float gl_ClipDistance[1];
};

void builtin_usage() {
    gl_ClipDistance[0] = 1.0;
    gl_Position = vec4(
        (gl_VertexIndex == 0) ? -4.0 : 1.0,
        (gl_VertexIndex == 2) ? 4.0 : -1.0,
        0.0,
        1.0
    );
}

void main()
{
    builtin_usage();
}

glslc test.vert -o test.spv
cargo r --bin naga test.spv

Entry point main at Vertex is invalid:
	Vertex shaders must return a `@builtin(position)` output value

Imberflur · 2024-02-11T21:02:12Z

If so, is it possible that later functions will be using the builtin but we remove the builtin label after not seeing any uses when processing the first function?

I'm reading the code further and I realized that this section of code only runs for the entry point function, so the issue is not quite as bad as I make it seem here. It seems like it's only an issue if the entry point itself doesn't access the builtin while another function that the entry point calls does access it.

since we probably shouldn't be naming things if they don't have a name in the input. As requested here: gfx-rs#5227 (comment)

Imberflur

Notes for additional context

naga/src/front/spv/function.rs

naga/tests/snapshots.rs

naga/tests/in/spv/unnamed-gl-per-vertex.spvasm

Imberflur · 2024-02-11T22:29:26Z

Would it be worth adding a test for this?

That would be great, thanks!

Added input spv to test for the original issue as well as another to test for the case of builtin's being accessed in a function parsed after the entry point.

This is a good point, we might need to defer the removal.

I ended up finding a way to defer this by postponing the entry point related processing to be after all function parsing. If this change is too much for this PR I can move it into a separate one.

This looks good, could we also take out the block below? I don't think we should be naming things if they don't have a name in the input.

Removed this block.

teoxoy

Great stuff, thanks!

teoxoy · 2024-02-15T16:36:18Z

One last thing, I would like to "rebase and merge" this PR instead of squashing it. Could you include the changes in the last commit ("fix typo") in the appropriate commits that introduced those?

handled even when this name is not in the input SPIRV.

since we probably shouldn't be naming things if they don't have a name in the input. As requested here: gfx-rs#5227 (comment)

…t function.

…are found before culling the unused ones

…entry point function

Imberflur · 2024-02-15T16:49:09Z

One last thing, I would like to "rebase and merge" this PR instead of squashing it. Could you include the changes in the last commit ("fix typo") in the appropriate commits that introduced those?

done

since we probably shouldn't be naming things if they don't have a name in the input. As requested here: #5227 (comment)

Imberflur requested a review from a team as a code owner February 9, 2024 02:55

Imberflur commented Feb 9, 2024

View reviewed changes

naga/src/front/spv/function.rs Outdated Show resolved Hide resolved

Imberflur changed the title ~~Remove unnecessary "gl_PerVertex" name check so unused builtins alway will be skipped~~ [naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins alway will be skipped Feb 9, 2024

Imberflur changed the title ~~[naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins alway will be skipped~~ [naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped Feb 9, 2024

Imberflur force-pushed the fix-unused-builtins branch 2 times, most recently from 0d40f54 to 10b3e98 Compare February 9, 2024 03:42

Imberflur force-pushed the fix-unused-builtins branch from 10b3e98 to 652b9ba Compare February 11, 2024 22:12

Imberflur force-pushed the fix-unused-builtins branch from 652b9ba to bf54971 Compare February 11, 2024 22:13

Imberflur commented Feb 11, 2024

View reviewed changes

naga/src/front/spv/function.rs Show resolved Hide resolved

naga/tests/snapshots.rs Show resolved Hide resolved

naga/tests/in/spv/unnamed-gl-per-vertex.spvasm Show resolved Hide resolved

teoxoy approved these changes Feb 15, 2024

View reviewed changes

Imberflur added 6 commits February 15, 2024 11:45

Remove unnecessary "gl_PerVertex" name check so unused builtins will be

2c018c3

handled even when this name is not in the input SPIRV.

[spv-in] Remove providing "perVertexStruct" for certain unnamed inputs

f070cac

since we probably shouldn't be naming things if they don't have a name in the input. As requested here: gfx-rs#5227 (comment)

Add test case to naga/spv-in for an unnamed glPerVertex struct

50f1966

Extract entry point processing from parse_function into an independen…

e13e1e4

…t function.

Defer entry-point processing so that all potential uses of builtin's …

2703bec

…are found before culling the unused ones

Add test case for builtin only accessed in function parsed after the …

bef7ac9

…entry point function

Imberflur force-pushed the fix-unused-builtins branch from 4e34662 to bef7ac9 Compare February 15, 2024 16:48

teoxoy merged commit abc0b30 into gfx-rs:trunk Feb 15, 2024
27 checks passed

teoxoy pushed a commit that referenced this pull request Feb 15, 2024

[spv-in] Remove providing "perVertexStruct" for certain unnamed inputs

4554f85

since we probably shouldn't be naming things if they don't have a name in the input. As requested here: #5227 (comment)

Imberflur deleted the fix-unused-builtins branch February 15, 2024 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped #5227

[naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped #5227

Imberflur commented Feb 9, 2024 •

edited

Loading

teoxoy commented Feb 9, 2024

teoxoy commented Feb 9, 2024

Imberflur commented Feb 11, 2024 •

edited

Loading

Imberflur commented Feb 11, 2024

Imberflur left a comment

Imberflur commented Feb 11, 2024 •

edited

Loading

teoxoy left a comment

teoxoy commented Feb 15, 2024

Imberflur commented Feb 15, 2024

[naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped #5227

[naga/spv-in] Remove unnecessary "gl_PerVertex" name check so unused builtins will always be skipped #5227

Conversation

Imberflur commented Feb 9, 2024 • edited Loading

teoxoy commented Feb 9, 2024

teoxoy commented Feb 9, 2024

Imberflur commented Feb 11, 2024 • edited Loading

Imberflur commented Feb 11, 2024

Imberflur left a comment

Choose a reason for hiding this comment

Imberflur commented Feb 11, 2024 • edited Loading

teoxoy left a comment

Choose a reason for hiding this comment

teoxoy commented Feb 15, 2024

Imberflur commented Feb 15, 2024

Imberflur commented Feb 9, 2024 •

edited

Loading

Imberflur commented Feb 11, 2024 •

edited

Loading

Imberflur commented Feb 11, 2024 •

edited

Loading